fix(toolkit-lib): clean up the diff change set when early validation fails by lemon0333 · Pull Request #1821 · aws/aws-cdk-cli

lemon0333 · 2026-08-12T05:08:35Z

When a cdk diff change set fails early validation (e.g. a resource that already exists), createChangeSetAndCleanup in toolkit-lib's cfn-api.ts threw from waitAndThrowOnProblem before the change-set and empty-stack cleanup could run. That orphaned the change set and left a new stack stuck in REVIEW_IN_PROGRESS, which then blocked subsequent change-set creation and forced the user to delete it manually in the console.

This change runs the cleanup on the failure path too — deleting the change set and, for a brand new stack, the empty review stack — as a best-effort step that does not mask the original validation error. Successful diffs are unchanged.

Verified with a new unit test in diff.test.ts (change set fails validation → DeleteChangeSet and DeleteStack are called); the full diff.test.ts suite passes (22/22).

Checklist

This change contains a major version upgrade for a dependency and I confirm all breaking changes are addressed
- Release notes for the new version:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…fails When a diff change set fails early validation (for example a resource that already exists), `waitAndThrowOnProblem` threw before the change-set and empty-stack cleanup ran, orphaning the change set and leaving a new stack stuck in REVIEW_IN_PROGRESS, which then blocked subsequent change-set creation on that stack. Run the cleanup on the failure path too (best-effort, without masking the original error) so a failed diff no longer leaks the change set or the empty review stack. Fixes aws#1767

ShadowCat567

Hello @lemon0333! Thanks for the PR :D it looks good overall! I just have a few comments that can help improve it

ShadowCat567 · 2026-08-13T19:15:33Z

    });
+  } catch (e) {
+    // Best-effort cleanup so a failed change set doesn't leak; don't let a
+    // cleanup failure mask the original creation/validation error.


Can you log the error we caught here? Currently the error that gets thrown will still not be visible to the user because it gets consumed here:

aws-cdk-cli/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts

Lines 74 to 92 in 536ad69

return await createChangeSetAndCleanup(ioHelper, {

cfn,

changeSetName: 'cdk-diff-change-set',

stack: options.stack,

exists,

uuid: options.uuid,

bodyParameter,

parameters: options.parameters,

resourcesToImport: options.resourcesToImport,

importExistingResources: options.importExistingResources,

includeNestedStacks: true,

role: executionRoleArn,

diagnoser,

});

} catch (e: any) {

// This function is currently only used by diff so these messages are diff-specific

if (options.failOnError) {

throw ToolkitError.withCause('ChangeSetCreationFailed', 'Could not create a change set, and \'--method=change-set\' was specified. Please check your permissions or use \'--method=auto\' to allow falling back to a template diff.', e);

}

(the actual error is only revealed if you use the --debug flag)

ShadowCat567 · 2026-08-13T19:21:22Z

+      await cleanup();
+    } catch (cleanupError) {
+      await ioHelper.defaults.debug(format('Failed to clean up change set after a creation error: %s', cleanupError));
+    }


having this try/catch may be overly defensive, I am not necessarily against it but why are we expecting that deletion might fail?

Address review: warn-log the caught error so the reason a diff change set failed is visible without -v (createDiffChangeSet otherwise only logs it at debug before falling back to a template diff), and elevate the best-effort cleanup failure log to warn. Assert the failure is surfaced in the test.

lemon0333 · 2026-08-14T14:37:08Z

Thanks for the review @ShadowCat567! Addressed both:

Visibility — the caught error is now warn-logged (Change set <id> failed: <reason>) so the reason is visible without -v, since createDiffChangeSet otherwise only consumes it at debug before falling back to the template diff.
The try/catch around cleanup — it's there so that a cleanup failure doesn't mask the original validation error (the useful one we want to rethrow). Deletion can legitimately fail — e.g. a role missing cloudformation:DeleteChangeSet/DeleteStack — and in that case we now warn-log the cleanup failure but still rethrow the original error. I added a comment making that rationale explicit; happy to simplify further if you'd prefer.

Also added a test assertion that the failure reason is surfaced. Full diff.test.ts passes (22/22).

ShadowCat567

Hello @lemon0333! Put some more thought into the try/catches, let me know what you think of my proposal

ShadowCat567 · 2026-08-14T15:31:56Z

-      StackName: changeSet.StackId ?? options.stack.stackName,
-      ClientRequestToken: randomUUID(),
+  let createdChangeSet: ChangeSetReport;
+  try {


I spent some more time thinking about this, I think the cleanest way to do this might be using a try/catch/finally:

try { return await new ChangeSetDescriber({...}); } catch (e) { await io.Helper.defaults.warn(...); // print the error throw e; } finally { // ensures we always clean up whether we ran into an error or not // do clean up, surface errors if any appear }

this means we are not calling the cleanup function in 2 places (and we probably don't need it to begin with). Let me know what you think!

Per review: move cleanup into a finally block so it runs once (not in two places) whether or not the change set succeeds, warn-log the failure reason in catch, and drop the extra defensive try/catch around cleanup.

lemon0333 · 2026-08-14T16:01:44Z

Nice, that's cleaner — done in the latest commit. Moved the cleanup into a finally so it runs once whether or not the change set succeeds, kept the warn in catch (which runs before finally, so the failure reason is surfaced even if cleanup itself errors), and dropped the extra defensive try/catch. diff.test.ts still passes (22/22). Thanks for the suggestion! 🙏

ShadowCat567

Nice job @lemon0333!

codecov-commenter · 2026-08-14T16:55:15Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.27%. Comparing base (536ad69) to head (85f4a42).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1821      +/-   ##
==========================================
- Coverage   90.32%   90.27%   -0.05%     
==========================================
  Files          80       80              
  Lines       12124    12124              
  Branches     1716     1714       -2     
==========================================
- Hits        10951    10945       -6     
- Misses       1139     1145       +6     
  Partials       34       34

Flag	Coverage Δ
suite.unit	`90.27% <ø> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lemon0333 deployed to automation August 12, 2026 05:08 — with GitHub Actions Active

github-actions Bot added bug p1 labels Aug 12, 2026

aws-cdk-automation requested a review from a team August 12, 2026 05:08

aws-cdk-automation enabled auto-merge August 12, 2026 05:08

lemon0333 requested a deployment to integ-approval August 12, 2026 05:08 — with GitHub Actions Waiting

ShadowCat567 reviewed Aug 13, 2026

View reviewed changes

auto-merge was automatically disabled August 14, 2026 14:37
Head branch was pushed to by a user without write access

lemon0333 requested a deployment to integ-approval August 14, 2026 14:37 — with GitHub Actions Waiting

ShadowCat567 self-assigned this Aug 14, 2026

ShadowCat567 reviewed Aug 14, 2026

View reviewed changes

refactor change set cleanup to try/catch/finally

85f4a42

Per review: move cleanup into a finally block so it runs once (not in two places) whether or not the change set succeeds, warn-log the failure reason in catch, and drop the extra defensive try/catch around cleanup.

lemon0333 had a problem deploying to integ-approval August 14, 2026 16:02 — with GitHub Actions Failure

ShadowCat567 approved these changes Aug 14, 2026

View reviewed changes

ShadowCat567 added this pull request to the merge queue Aug 14, 2026

Merged via the queue into aws:main with commit 750d650 Aug 14, 2026
16 of 17 checks passed

Navigation Menu

Uh oh!

fix(toolkit-lib): clean up the diff change set when early validation fails - #1821

fix(toolkit-lib): clean up the diff change set when early validation fails#1821
ShadowCat567 merged 3 commits into
aws:mainfrom
lemon0333:fix/diff-changeset-cleanup-on-validation-failure

lemon0333 commented Aug 12, 2026

Uh oh!

ShadowCat567 left a comment

Uh oh!

ShadowCat567 Aug 13, 2026

Uh oh!

ShadowCat567 Aug 13, 2026

Uh oh!

lemon0333 commented Aug 14, 2026

Uh oh!

ShadowCat567 left a comment

Uh oh!

ShadowCat567 Aug 14, 2026

Uh oh!

lemon0333 commented Aug 14, 2026

Uh oh!

ShadowCat567 left a comment

Uh oh!

codecov-commenter commented Aug 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	return await createChangeSetAndCleanup(ioHelper, {
	cfn,
	changeSetName: 'cdk-diff-change-set',
	stack: options.stack,
	exists,
	uuid: options.uuid,
	bodyParameter,
	parameters: options.parameters,
	resourcesToImport: options.resourcesToImport,
	importExistingResources: options.importExistingResources,
	includeNestedStacks: true,
	role: executionRoleArn,
	diagnoser,
	});
	} catch (e: any) {
	// This function is currently only used by diff so these messages are diff-specific
	if (options.failOnError) {
	throw ToolkitError.withCause('ChangeSetCreationFailed', 'Could not create a change set, and \'--method=change-set\' was specified. Please check your permissions or use \'--method=auto\' to allow falling back to a template diff.', e);
	}

Uh oh!

Conversation

lemon0333 commented Aug 12, 2026

Checklist

Uh oh!

ShadowCat567 left a comment

Choose a reason for hiding this comment

Uh oh!

ShadowCat567 Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

ShadowCat567 Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

lemon0333 commented Aug 14, 2026

Uh oh!

ShadowCat567 left a comment

Choose a reason for hiding this comment

Uh oh!

ShadowCat567 Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

lemon0333 commented Aug 14, 2026

Uh oh!

ShadowCat567 left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 14, 2026

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants